nfd-worker: Watch features.d changes #2156

ozhuraki · 2025-05-12T19:05:35Z

Closes: #2075

netlify · 2025-05-12T19:05:41Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`b9ea21a`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-nfd/deploys/689df2d5da4e1600083e3e28
😎 Deploy Preview	https://deploy-preview-2156--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

k8s-ci-robot · 2025-05-12T19:05:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ozhuraki
Once this PR has been reviewed and has the lgtm label, please assign marquiz for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-05-12T19:05:45Z

Hi @ozhuraki. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

marquiz

Thanks @ozhuraki for taking a stab at this.

I think we should refactor the code and re-architecture this more to make the code more maintainable. There might be other sources we'd also make react to events in a similar way. Basically, it should be the source (source/local in this case) which should be able to notify the main event loop that features have been updated. Also, no need to run re-discovery of all features.

ozhuraki · 2025-05-13T08:55:12Z

@marquiz

Thanks, makes sense. I will move this into source/local.

ozhuraki · 2025-05-22T14:01:31Z

@marquiz

Moved into source/local, please take a look

ozhuraki · 2025-06-03T11:11:43Z

@ArangoGutierrez

Thanks, updated, please take a look.

marquiz

Progress in the right direction, but I maintain that we should aim for a more generic, maintainable solution. For example, who knows in the future we might want to do some uevent-based stuff or similar and it would be good to have the basics right for that, instead building of pile of one-off tricks.

Some specific observations:

We operate on interfaces in nfd-worker, IMO we better keep that to keep the design cleaner. E.g. introduce a new AsyncSource, EventSource or smth with a method to set the event channel, and then when configuring/enabling the feature sources check if the source implements the interface and if it does call the method
It should be the source/local who is internally setting up the the fswatcher and notifies nfd-worker. Then, we have two possibilities here:
- either nfd-worker does the source.Discover() and then advertises the updated features/labels
- or the source runs discovery internally and notifies nfd-worker just to re-advertise update features
When a source notifies the nfd-worker main loop, the main loop does not need to do full re-discovery of all feature sources
Some unit test for the local source would be nide 😊

source/local/local.go

marquiz · 2025-06-25T12:49:02Z

source/local/local.go

+	if err != nil {
+		if !os.IsNotExist(err) {
+			return err
+		}


If it does not exist we probably want to exit, too (return nil)?

source/local/local.go

source/source.go

pkg/nfd-worker/nfd-worker.go

marquiz · 2025-07-29T10:09:43Z

ping @ozhuraki any update on this?

Signed-off-by: Oleg Zhurakivskyy <[email protected]>

ArangoGutierrez · 2025-08-14T12:39:57Z

/ok-to-test

ArangoGutierrez · 2025-08-14T12:47:22Z

/test pull-node-feature-discovery-verify-master

ArangoGutierrez · 2025-08-14T14:41:50Z

/retest

Copilot

Pull Request Overview

This PR implements filesystem watching functionality for the nfd-worker to automatically detect changes in the features.d directory. It introduces an EventSource interface to enable sources to send notifications when their underlying data changes, specifically targeting the local source to watch for file modifications.

Adds EventSource interface and related infrastructure for event-driven feature discovery
Implements filesystem watching in the local source using fsnotify
Integrates event handling into the nfd-worker main loop to trigger selective feature discovery

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
source/source.go	Adds EventSource interface and GetAllEventSources() function
source/local/local.go	Implements EventSource interface with fsnotify-based file watching
pkg/nfd-worker/nfd-worker.go	Integrates event handling and selective feature discovery by source

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.}

Copilot · 2025-08-14T14:43:43Z

source/local/local.go

+				ch <- s.Name()
+			}
+		case err := <-s.fsWatcher.Errors:
+			klog.ErrorS(err, "failed to to watch features.d changes")


There's a duplicate 'to' in the error message. It should be "failed to watch features.d changes".

Suggested change

klog.ErrorS(err, "failed to to watch features.d changes")

klog.ErrorS(err, "failed to watch features.d changes")

Copilot · 2025-08-14T14:43:44Z

source/local/local.go

+		case err := <-s.fsWatcher.Errors:
+			klog.ErrorS(err, "failed to to watch features.d changes")
+		}
+		time.Sleep(1 * time.Second)


The hardcoded 1-second sleep in the event loop may cause unnecessary delays in event processing. Consider removing this sleep or making it configurable, as fsnotify events should be processed immediately.

Suggested change

time.Sleep(1 * time.Second)

Copilot · 2025-08-14T14:43:44Z

source/local/local.go

+	}
+
+	go s.runNotifier(ch)
+


The goroutine is started unconditionally, even when no filesystem watcher is created (when the directory doesn't exist). This will create a goroutine that only handles errors from a nil watcher, which could lead to unexpected behavior.

Suggested change

go s.runNotifier(ch)

}

Copilot · 2025-08-14T14:43:45Z

pkg/nfd-worker/nfd-worker.go

@@ -121,6 +121,7 @@ type nfdWorker struct {
 	k8sClient           k8sclient.Interface
 	nfdClient           nfdclient.Interface
 	stop                chan struct{} // channel for signaling stop
+	sourceEvent         chan string   // channel for events from soures


There's a spelling error in the comment. 'soures' should be 'sources'.

Suggested change

sourceEvent chan string // channel for events from soures

sourceEvent chan string // channel for events from sources

Copilot · 2025-08-14T14:43:45Z

pkg/nfd-worker/nfd-worker.go

+	w.sourceEvent = make(chan string)
+	eventSources := source.GetAllEventSources()
+	for _, s := range eventSources {
+		s.SetNotifyChannel(w.sourceEvent)


The error returned by SetNotifyChannel() is not being handled. If setting up the notification channel fails for any source, it could lead to silent failures in event monitoring.

Suggested change

s.SetNotifyChannel(w.sourceEvent)

if err := s.SetNotifyChannel(w.sourceEvent); err != nil {

klog.ErrorS(err, "failed to set notify channel for event source", "source", s.Name())

return fmt.Errorf("failed to set notify channel for event source %s: %w", s.Name(), err)

}

k8s-ci-robot · 2025-08-14T16:31:40Z

@ozhuraki: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-node-feature-discovery-verify-master	`b9ea21a`	link	true	`/test pull-node-feature-discovery-verify-master`
pull-node-feature-discovery-e2e-test-master	`b9ea21a`	link	true	`/test pull-node-feature-discovery-e2e-test-master`
pull-node-feature-discovery-build-image-cross-generic	`b9ea21a`	link	true	`/test pull-node-feature-discovery-build-image-cross-generic`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 12, 2025

k8s-ci-robot requested review from ArangoGutierrez and marquiz May 12, 2025 19:05

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 12, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 12, 2025

marquiz reviewed May 13, 2025

View reviewed changes

ozhuraki force-pushed the watch-featuresd branch from 89606c0 to b3b1c64 Compare May 22, 2025 13:32

ArangoGutierrez requested a review from Copilot May 28, 2025 05:58

This comment was marked as outdated.

Sign in to view

ozhuraki force-pushed the watch-featuresd branch from b3b1c64 to 9a65598 Compare June 3, 2025 11:06

marquiz reviewed Jun 5, 2025

View reviewed changes

ozhuraki force-pushed the watch-featuresd branch from 9a65598 to 7c29f0a Compare June 18, 2025 09:27

marquiz reviewed Jun 25, 2025

View reviewed changes

nfd-worker: Watch features.d changes

b9ea21a

Signed-off-by: Oleg Zhurakivskyy <[email protected]>

ArangoGutierrez requested a review from Copilot August 14, 2025 12:39

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 14, 2025

This comment was marked as outdated.

Sign in to view

ozhuraki force-pushed the watch-featuresd branch from 7c29f0a to b9ea21a Compare August 14, 2025 14:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 14, 2025

ArangoGutierrez requested review from Copilot and marquiz August 14, 2025 14:36

Copilot AI reviewed Aug 14, 2025

View reviewed changes

	klog.ErrorS(err, "failed to to watch features.d changes")
	klog.ErrorS(err, "failed to watch features.d changes")

	sourceEvent chan string // channel for events from soures
	sourceEvent chan string // channel for events from sources

-		s.SetNotifyChannel(w.sourceEvent)
+		if err := s.SetNotifyChannel(w.sourceEvent); err != nil {
+			klog.ErrorS(err, "failed to set notify channel for event source", "source", s.Name())
+			return fmt.Errorf("failed to set notify channel for event source %s: %w", s.Name(), err)
+		}

nfd-worker: Watch features.d changes #2156

Are you sure you want to change the base?

nfd-worker: Watch features.d changes #2156

Conversation

ozhuraki commented May 12, 2025

Uh oh!

netlify bot commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Uh oh!

k8s-ci-robot commented May 12, 2025

Uh oh!

k8s-ci-robot commented May 12, 2025

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

ozhuraki commented May 13, 2025

Uh oh!

ozhuraki commented May 22, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

ozhuraki commented Jun 3, 2025

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marquiz Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marquiz commented Jul 29, 2025

Uh oh!

ArangoGutierrez commented Aug 14, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

ArangoGutierrez commented Aug 14, 2025

Uh oh!

ArangoGutierrez commented Aug 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 14, 2025

Uh oh!

Uh oh!

netlify bot commented May 12, 2025 •

edited

Loading